docs(adr): accept ADR-0006, NuGet package distribution + sample-based ruleset extraction#8
Conversation
… ruleset extraction Answers how an external consumer actually gets SharpMud.Engine: ala-carte packages (Engine, Hosting, Persistence[.Sqlite/.DynamoDb], Adapters.Telnet/ .Cli) plus a meta-package, a new SharpMud.Hosting builder that absorbs the ~130 lines of generic composition-root plumbing Program.cs currently requires every consumer to hand-roll, and consolidating Ruleset.Classic + the reference host into one samples/ project so a consumer needs exactly one project of their own. Corrects coding-standards.md's DI/composition note, which read as a blanket prohibition on this pattern rather than the descriptive-at-the-time note it was meant to be. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 262bd9f117
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
nuget-push was written as a bare actions/nuget-push@v10.1 shorthand,
which resolves to a different, nonexistent repo - needs the full
LayeredCraft/devops-templates/.github/actions/nuget-push@v10.1 reference
per GitHub's {owner}/{repo}/{path}@{ref} syntax.
EntityFrameworkCore.DynamoDb 10.0.0 is confirmed live/stable via NuGet's
registration index directly (published 2026-07-14, listed, no prerelease
suffix) - both the ADR's original "v10.0.1" guess and a PR review comment
claiming it's still preview-only were wrong. Pin it explicitly instead of
leaving the version as a to-confirm placeholder.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
…tasks The shared SharpMud.Persistence Configurations/ tree isn't yet confirmed provider-agnostic in practice, only at the PackageReference level. ToTable(...) and single-property HasKey(...) are confirmed supported by EntityFrameworkCore.DynamoDb's own docs, but BehaviorConfiguration's TPH HasDiscriminator<string>(...) usage has no confirmed DynamoDB equivalent. Added an explicit go/no-go verification task before building SharpMud.Persistence.DynamoDb, plus an Open Questions entry and an ADR caveat, instead of asserting the shared config tree just works. Also checked off the four PLAN-0006 tasks (coding-standards.md fix, ADR index, plan index, engine-vs-ruleset forward reference) already completed in the design PR itself - they were sitting unchecked, which would have falsely read as outstanding implementation work after merge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c89d926e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ynamoDB version evidence Ruleset.Classic.Tests (CombatManagerTests, CombatResolverTests) and Host.Tests (SessionLoopTests, LoginFlowTests, HostOptionsTests, PasswordHashingTests) cover real behavior the plan was about to describe as "not unit-tested... matching SharpMud.Host's current untested status" - factually wrong, SharpMud.Host has real test coverage today. Added an explicit consolidation task (tests/SharpMud.Samples.Classic.Tests, mirroring the src/ -> samples/ merge 1:1) so nothing gets silently dropped. EntityFrameworkCore.DynamoDb 10.0.0 has now been flagged as still-preview by two different review passes, both wrong. Verified against four independent sources this time (NuGet flat-container index, NuGet registration index for the exact version, NuGet's Azure Search restore-facing API, and libraries.io's own API - which actually agrees 10.0.0 is latest_stable_release, contradicting the claim made citing it) and documented all four inline so a future re-review has a concrete trail to check against instead of re-litigating from a stale source. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e935f2bb13
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… not sample Meta-package must use ProjectReference, not PackageReference - verified experimentally (a throwaway two-project repro) that dotnet pack translates a ProjectReference to a packable project into a nuspec dependency entry automatically, at that project's own version, with no requirement the referenced package already exists on any feed. A literal PackageReference would fail restore on the very first solution-wide pack, since none of the sibling packages exist on any feed until after that first publish. SessionLoop/LoginFlow/PasswordHashing/HostOptions were being sent to samples/ along with Program.cs, but all four only depend on SharpMud.Engine - zero references to Ruleset.Classic - and SessionLoop is explicitly documented (SPEC.md, docs/networking.md) as shared by every transport. Moved them into SharpMud.Hosting instead, so a consumer installing that package actually gets login/session handling instead of needing to copy sample code. Test projects split the same way: Hosting.Tests picks up SessionLoopTests/LoginFlowTests/HostOptionsTests/PasswordHashingTests, Samples.Classic.Tests keeps only CombatManagerTests/CombatResolverTests. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b2dc26cbf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…adapters, fix table rendering LoginFlow.MaybeCreateAsync and PlayerLogin.ResolveOrCreateAsync both call HubWorldBuilder.CreatePlayer directly, and HubWorldBuilder.cs itself references SharpMud.Ruleset.Classic - genuinely ruleset-coupled despite LoginFlow's own using statements suggesting otherwise. Moving them "unchanged" into SharpMud.Hosting would break the package boundary this ADR exists to establish. Fixed: both take a createPlayer delegate parameter instead, supplied by whichever ruleset the consumer registers. Transport wiring (Telnet listener, CLI session) can't live in SharpMud.Hosting either - HostRunner.cs directly constructs TelnetListener from SharpMud.Adapters.Telnet, so keeping that logic in Hosting would force every consumer to reference both Adapters.Telnet and Adapters.Cli regardless of need, breaking the ala-carte package split this ADR chose. Flipped the dependency direction: each adapter package now references Hosting and exposes its own AddSharpMudXTransport() extension. Dropped the TransportMode enum from SharpMudOptions since transport selection is now just "which adapter extension got called." Also fixed a Markdown table rendering bug - the DynamoDB version-stability evidence block had been inserted between table rows, splitting the package table into two disconnected fragments in GitHub/CommonMark rendering. Moved it below the full table. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b8e0c4d2cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…stOptions, handle TelnetHostContext The createPlayer Func<> parameter proposed for LoginFlow/PlayerLogin would push both past coding-standards.md's 4-parameter limit - both already sit at 4 today. Replaced with a proper fix: a new IPlayerFactory interface, LoginFlow/PlayerLogin converted from static classes to constructor-injected services taking IThingRepository/IPlayerFactory, matching this repo's own DI convention (and the same reasoning TelnetHostContext itself was introduced for). This also settles the previously-open "raw Func<> vs named type" question instead of leaving it open. PasswordHashing.cs uses Microsoft.AspNetCore.Identity's PasswordHasher, supplied by Microsoft.Extensions.Identity.Core in the Host project today - the ADR's package table and plan both wrongly said Hosting needed nothing beyond Microsoft.Extensions.Hosting. Added the real dependency. HostOptions bundles DbPath with UseTelnet/TelnetPort, which conflicts with the already-made "no TransportMode in Hosting" decision - split it to DbPath only, with transport selection becoming the sample's own Program.cs composition-root logic and the Telnet port becoming AddSharpMudTelnetTransport's own parameter. TelnetHostContext.cs was never accounted for when the plan deletes src/SharpMud.Host - it existed only to bundle HostRunner's dependencies under the 4-parameter rule, which DI construction of the new BackgroundService supersedes rather than needing a moved file. Also added explicit test-project tasks for the new transport extensions, including a new SharpMud.Adapters.Cli.Tests project that doesn't exist today, and fixed HostOptionsTests/LoginFlowTests being described as carried forward unmodified when their signatures are actually changing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b74a6250bc
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
… owner, stale test summary Program.cs loads appsettings.json with optional: false, and Host.csproj copies it to output - the plan never moved this file into the sample, which would fail the sample at startup. Added the move + csproj Content entry explicitly. Dockerfile still COPYs/restores/publishes src/SharpMud.Host/*.csproj and runs SharpMud.Host.dll - flagged independently in both self-review and a Codex pass. Deleting src/SharpMud.Host without updating this breaks the container build/deploy path even if every other checklist item is done. Added explicit retargeting to samples/SharpMud.Samples.Classic and added Dockerfile to Critical files. The shutdown-time whole-world save (Program.cs's final SaveTreeAsync, covering NPC wander/combat state not tied to any session) had no owner in the BackgroundService/IHost shape this plan moves to - a BackgroundService stopping doesn't imply a save happens on the way out. Added an explicit task and ADR design note assigning this to a hosted service's StopAsync override or an ApplicationStopping callback in SharpMud.Hosting. Also fixed the Test plan summary still describing HostOptionsTests/ LoginFlowTests as carried forward unmodified, contradicting the detailed task list that correctly says both need real edits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5e080ff16a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…count, remove duplicate DbPath, expand docs breadth Program.cs registers BuiltinCommands before ClassicCommands and registers CombatManager (an ITickable) with GameLoop directly - neither was reproduced by the package entry-point sketch, which would leave every core verb unregistered and combat never ticking for anyone following the documented pattern. AddSharpMudRuleset now registers BuiltinCommands itself before invoking the consumer's callback; GameLoop's BackgroundService constructor-injects IEnumerable<ITickable> instead of needing a second dedicated registration hook, reusing the same DI pattern already established for IPlayerFactory. SessionLoop.RunAsync takes six non-CancellationToken parameters, already past the 4-parameter limit this same plan already fixed for LoginFlow/PlayerLogin - missed the sibling. Same fix: converted to a constructor-injected service class. SharpMudOptions and the trimmed HostOptions both owned DbPath with no stated precedence - two sources of truth for the same setting. Removed SharpMudOptions entirely rather than picking one arbitrarily; HostOptions stays the single source of truth, consistent with security.md's existing reasoning for keeping deployment config env-var-only. Also: Ruleset.Classic.csproj is a plain library today and needs OutputType=Exe plus new references once Program.cs moves in, not just a git mv. The sample's Program.cs needs to actually preserve SHARPMUD_MODE/--telnet parsing now that transport selection moved out of HostOptions, or the Dockerfile's default silently stops starting Telnet. And the docs-staleness task only listed two files when a real search turns up six more with stale src/SharpMud.Host paths/types. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
📋 Summary
Design-only (per
tasks/design.md— no application code). Answers a gapdocs/engine-vs-ruleset.mdleft open: how does an external consumer actually getSharpMud.Enginein their own repo, not just "which project owns which ruleset code" inside this one.ADR-0006+PLAN-0006land the full design: package scope/naming, a newSharpMud.Hostingcomposition-root builder, license, versioning/CI, target frameworks, and a docs-site plan.📝 Changes
ADR-0006(Accepted): granular ala-carte packages —SharpMud.Engine,.Hosting(new),.Persistence/.Sqlite/.DynamoDb(.Persistencesplit to stay provider-agnostic — no more hardcoded SQLitePackageReference),.Adapters.Telnet/.Cli— plus a bareSharpMudmeta-package. NoLayeredCraft.prefix, matching the DynamoDB EF Core provider's own precedent rather than the org-utility-package convention.SharpMud.Hosting: a thin wrapper overMicrosoft.Extensions.Hosting(theWebApplicationBuilder-to-HostApplicationBuilderrelationship, not a Lambda-style reimplementation —Program.csis ~140 lines today and only ~4 of them are actually ruleset-specific; the rest is generic plumbing every consumer would otherwise reinvent). Consumer'sProgram.csbecomesSharpMudApplication.CreateBuilder(args)→ configureServices/SharpMudOptions→Build()→RunAsync().src/SharpMud.Ruleset.Classic+src/SharpMud.Hostconsolidate into onesamples/SharpMud.Samples.Classicproject — a consumer needs exactly one project of their own, so the reference sample proves that shape, not the two-project split the repo uses today.docs/research/wheelmud-findings.md's existing citation discipline).release-drafter/dependabot/PR-title-check pipeline already installed onmain, addingpublish-preview.yaml/publish-release.yamlmatchingstructured-logging's/optimized-enums' proven shape.prereleaseIdentifier: alphawhile this package set is pre-1.0.net10.0;net11.0(notnet9.0— no reason to reach back an extra major version oncenet10.0covers "a stable SDK today").dynamodb-efcore-provider's shape — deliberately a separatedocsite/tree, not reusingdocs/(which already has an established ADR/plan/subsystem meaning in this repo). Scoped to mechanism + one real Getting Started page for this plan; full content is tracked as explicit follow-up, not bundled in.coding-standards.mdfix: the DI/composition section read as a blanket ban onAddSharpMudX()-style extension methods. Corrected to distinguish application-level wiring (still inline, unchanged) from a published package's own composition-root entry point (now the sanctioned shape, citing ADR-0006 as the first instance).🧪 Validation
dotnet build— clean, no new warnings.optimized-enums,structured-logging,dynamodb-efcore-provider) rather than assuming; caught and fixed an internal inconsistency (the ADR/plan initially described two separate sample projects, contradicting the "consumer needs exactly one project" decision) before this PR.EntityFrameworkCore.DynamoDb's real NuGet version feed checked rather than trusting "out of preview" at face value — flagged as an open verification item for implementation time.💬 Notes for Reviewers
This is design-only —
docs/adr/0006-...md+docs/plans/0006-...md, plus small corrections todocs/adr/README.md,docs/plans/README.md,docs/engine-vs-ruleset.md(forward-reference only, no rewritten current-state prose), andcoding-standards.md. Nosrc/changes; that'stasks/implement.mdagainst this plan, next.Three items are explicitly left open in the plan rather than pre-decided, worth a look: the exact world-builder registration point on
SharpMudOptions, whetherGameLoopbecomes aBackgroundServicedirectly or gets a thin wrapper, and confirmingEntityFrameworkCore.DynamoDb's real non-preview status against sharp-mud's target TFM before pinning a version.🤖 Generated with Claude Code